Skip to content

feat: use getAsFileSystemHandle when available #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 10, 2024
Merged

feat: use getAsFileSystemHandle when available #91

merged 2 commits into from
Oct 10, 2024

Conversation

@rolandjitsu
Copy link
Collaborator

@rolandjitsu rolandjitsu commented Oct 8, 2024

What kind of change does this PR introduce?

  • Bug Fix
  • Feature
  • Refactoring
  • Style
  • Build
  • Chore
  • Documentation
  • CI

Did you add tests for your changes?

  • Yes, my code is well tested
  • Not relevant

If relevant, did you update the documentation?

  • Yes, I've updated the documentation
  • Not relevant

Summary

Complete the impl. of #79 with tests and typescript types.

Does this PR introduce a breaking change?

No.

Other information

@rolandjitsu rolandjitsu self-assigned this Oct 8, 2024
@rolandjitsu
Copy link
Collaborator Author

rolandjitsu commented Oct 8, 2024

@tomayac please check this PR out. I've added some tests and additional typescript type support for the new handle property.

@coveralls
Copy link

coveralls commented Oct 8, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build ae387457db4eb1e92b2d2a815de8bc263fd43dd2-PR-91

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 6adbc2fe4dad708280f8bc60590335a892510731: 0.0%
Covered Lines: 89
Relevant Lines: 89
💛 - Coveralls

rolandjitsu
rolandjitsu commented Oct 8, 2024
View reviewed changes
@rolandjitsu rolandjitsu force-pushed the feat/fs-handle branch 2 times, most recently from 254b192 to cc7ca27 Compare October 8, 2024 19:29
tomayac
tomayac approved these changes Oct 9, 2024
View reviewed changes
Copy link

@tomayac tomayac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'm not sure if it matters, but you may want to replace FileSystemHandle with the more explicit FileSystemFileHandle, since this is only dealing with files, not directories (for which FileSystemDirectoryHandle exists, both inherit from FileSystemHandle).

src/file-selector.spec.ts Outdated
const name = 'test.json';
const [f, h] = createFileSystemFileHandle(name, {ping: true}, {
type: 'application/json'
})
Copy link

@tomayac tomayac Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
})
});

Copy link
Collaborator Author

@rolandjitsu rolandjitsu Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this.

@rolandjitsu
Copy link
Collaborator Author

rolandjitsu commented Oct 9, 2024

LGTM

I'm not sure if it matters, but you may want to replace FileSystemHandle with the more explicit FileSystemFileHandle, since this is only dealing with files, not directories (for which FileSystemDirectoryHandle exists, both inherit from FileSystemHandle).

Made the handle of type FileSystemFileHandle on FileWithPath and kept the internal use as FileSystemHandle.

@rolandjitsu
Copy link
Collaborator Author

rolandjitsu commented Oct 9, 2024

@tomayac can we go ahead with this PR or would you like to update yours to match this one?

@tomayac
Copy link

tomayac commented Oct 9, 2024

@tomayac can we go ahead with this PR or would you like to update yours to match this one?

No worries with taking yous. Just close mine in favor of this. Glad the feature is landing.

@rolandjitsu rolandjitsu merged commit 7e3cef5 into master Oct 10, 2024
8 checks passed
@github-actions
Copy link

github-actions bot commented Oct 10, 2024

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rolandjitsu rolandjitsu mentioned this pull request Oct 10, 2024
Closed
12 tasks
@rolandjitsu rolandjitsu deleted the feat/fs-handle branch November 2, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tomayac tomayac tomayac approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rolandjitsu rolandjitsu

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rolandjitsu @coveralls @tomayac